fix: memory leak in the webhook TLS healthcheck#2690
Merged
sozercan merged 2 commits intoApr 14, 2023
Conversation
dethi
force-pushed
the
tls-webhook-healtcheck-memory-leak
branch
from
648f541 to
d98533c
Compare
dethi
changed the title
- The resp.Body was never closed, thus causing one connection to be leaked for each executions. - Creating a new transport based on the default transport, to inherit some of the default timeouts. Most importantly, this ensure that there is a default TLSHandshakeTimeout (10s) and dial timeout (30s). - Disable http keep alive, to avoid reuse of the same http connection. Otherwise, we may fail the healthcheck when the certs is rotated (new cert on disk wouldn't match the cert attached to the reused connection opened earlier). Fix open-policy-agent#2654 Signed-off-by: Thibault Deutsch <thibault@arista.com>
dethi
force-pushed
the
tls-webhook-healtcheck-memory-leak
branch
from
d98533c to
69300f5
Compare
acpana
reviewed
Apr 10, 2023
Contributor
acpana
left a comment
acpana
left a comment
There was a problem hiding this comment.
(first, thanks for all the engagement on the issue and for opening a PR 💯 )
quick question re keep alives;
Comment on lines
+23
to
+25
| // disable keep alives to ensure that http connection aren't reused, otherwise the check may | ||
| // fail if the cert was rotated in between | ||
| tr.DisableKeepAlives = true |
Contributor
There was a problem hiding this comment.
would keep alives here just cause some network flakiness? iow, if checks retry once the certs have been rotated, would this still be a problem?
Contributor
Author
There was a problem hiding this comment.
I don't think it would be just flakiness. I didn't get the time to validate my theory, but here is the scenario that I'm thinking (with keepalive enabled):
- First health check pass, with certificate A. Because we drained the body before closing it, the connection is kept open (that's the default behaviour of the Go HTTP client, as long as the server also allow keepalive)
- Then, the certificate is renewed. Certificate A is replaced by certificate B on file, and the transport layer of the HTTP server now use certificate B for new connections.
- A new health check is started. The previous connection is open, in the pool of connections. The Go HTTP client select it and make a new request. We get a response. When we check the response, we compare what we have on disk (certificate B) with the peer cert associated to the connection (certificate A). The check fail.
- This repeat indefinitely until the connection is closed, which may never happen (or after X minutes when the maximum connection lifetime is exceeded), because we are always properly draining and closing the body, so the HTTP client should always keep the connection open.
Contributor
There was a problem hiding this comment.
That seems likely. IIRC certs are only used for negotiating a session key, so rotating a cert wouldn't necessarily break a pre-existing connection.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2690 +/- ##
==========================================
- Coverage 53.27% 52.72% -0.55%
==========================================
Files 120 123 +3
Lines 10594 10941 +347
==========================================
+ Hits 5644 5769 +125
- Misses 4515 4715 +200
- Partials 435 457 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
maxsmythe
approved these changes
Apr 10, 2023
Contributor
maxsmythe
left a comment
maxsmythe
left a comment
There was a problem hiding this comment.
LGTM, thank you for finding/fixing this!
sozercan
approved these changes
Apr 14, 2023
davis-haba
pushed a commit
to davis-haba/gatekeeper
that referenced
this pull request
Apr 18, 2023
Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
sozercan
added a commit
that referenced
this pull request
Apr 25, 2023
salaxander
pushed a commit
to salaxander/gatekeeper
that referenced
this pull request
Apr 27, 2023
Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: Xander Grzywinski <xandergr@microsoft.com>
This was referenced Nov 22, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The resp.Body was never closed, thus causing one connection to be leaked for each executions.
Creating a new transport based on the default transport, to inherit some of the default timeouts. Most importantly, this ensure that there is a default TLSHandshakeTimeout (10s) and dial timeout (30s).
Disable http keep alive, to avoid reuse of the same http connection. Otherwise, it may fail the check when the certs is rotated (new cert on disk wouldn't match the cert attached to the reused connection opened earlier).
Which issue(s) this PR fixes
Fixes #2654
Special notes for your reviewer: